EFA GDA: a Put always posts at least one WQE - #43
Merged
akkart-aws merged 1 commit intoAug 4, 2026
Conversation
putImplMode skipped the post entirely for a put with no payload and no signal/counter (hasPayload || needsSignalEp gate): with nothing to deliver and nothing to tick, the operation looked like a no-op and was silently dropped. That was an as-if optimization from before request aggregation existed, and aggregation broke its premise. With ncclGinOptFlagsAggregateRequests, a put carries a LOCAL side effect: its non-aggregated doorbell rendezvous in postRdmaWrite is what publishes earlier deferred WQEs on the QP. Dropping "empty" puts therefore made a deferred stream impossible to terminate when its last real put happened to be aggregated -- the tail WQEs (and their signals) were never handed to the NIC and the peer waited forever. Change the contract so a Put always posts at least one WQE. The empty case degenerates to a 0-byte write to the peer's per-context scratch via the peer DATA endpoint (target slot 0), which binds no FI_REMOTE_WRITE -- remotely unobservable; locally the doorbell rendezvous runs and submitted_count/FI_WRITE tick in lockstep, so Flush accounting stays exact. Signed-off-by: Arun Karthik <akkart@amazon.com>
anshumang
reviewed
Aug 3, 2026
anshumang
left a comment
anshumang
left a comment
Collaborator
There was a problem hiding this comment.
With aggregate flag in put, flush needs to ring doorbell to guarantee that the sender's buffers can be reused. I think we should add a doorbell ring in flush which is already in the slow path than add an extra doorbell ring in the fast path, that is, put. Say, the user calls put with zero payload + counter + signal + aggregate off twice. Ringing the doorbell in the first call is required, what this PR addresses. But the ringing in the second call is redundant and adds latency. Although, this seems like a sub-optimally use of EFA GDA at the user level, but better to make the user pay the penalty with suboptimal flush than suboptimal put.
anshumang
approved these changes
Aug 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
putImplMode skipped the post entirely for a put with no payload and no signal/counter (hasPayload || needsSignalEp gate): with nothing to deliver and nothing to tick, the operation looked like a no-op and was silently dropped.
That was an as-if optimization from before request aggregation existed, and aggregation broke its premise. With
ncclGinOptFlagsAggregateRequests, a put carries a LOCAL side effect: its non-aggregated doorbell rendezvous in postRdmaWrite is what publishes earlier deferred WQEs on the QP. Dropping "empty" puts therefore made a deferred stream impossible to terminate when its last real put happened to be aggregated -- the tail WQEs (and their signals) were never handed to the NIC and the peer waited forever.
Change the contract so a Put always posts at least one WQE. The empty case degenerates to a 0-byte write to the peer's per-context scratch via the peer DATA endpoint (target slot 0), which binds no FI_REMOTE_WRITE -- remotely unobservable; locally the doorbell rendezvous runs and submitted_count/FI_WRITE tick in lockstep, so Flush accounting stays exact.